Skip to content

Conversation

MozirDmitriy
Copy link

@MozirDmitriy MozirDmitriy commented Jun 24, 2025

Description

This pull request addresses #2091 Documentation Cleanup by reorganizing and improving the project documentation, enhancing cross-references, and automating the deployment of Rust API documentation.
Key changes include:

  • Reorganized and clarified the structure of the docs/ folder, adding cross-links between the main README and documentation index.
  • Updated docs/README.md and docs/vm/README.md to improve navigation and reference benchmark results.
  • Added links to benchmark results and extended documentation in the main README.md.
  • Enhanced vm/src/lib.rs with module-level rustdoc, including links to the main README and documentation, and mentioning mermaid diagram support.
  • Added the aquamarine dev-dependency to enable mermaid diagrams in rustdoc.
  • Updated the GitHub Actions workflow (.github/workflows/bench.yml) to build and deploy Rust API documentation (cargo doc) to GitHub Pages alongside benchmark results.
    These changes make the documentation more discoverable, up-to-date, and easier to maintain, while also ensuring that both benchmarks and API docs are always available online.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

Copy link
Collaborator

@gabrielbosio gabrielbosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the cross-links and links to benchmarks are useful, this PR doesn't address the corresponding issue:

  • This PR doesn't clean up or reorganize documentation.
  • It doesn't create a docs.rs following Cairo Native docs as a reference.
  • While the aquamarine dependency is added, it is not used.

I think the corresponding issue has to be divided into sub issues so we avoid having a huge PR that attempts to address this. What do you think?

make hyper-threading-benchmarks
```

Benchmark results are available in [docs/benchmarks/criterion_benchmark.pdf](docs/benchmarks/criterion_benchmark.pdf) and [docs/benchmarks/flamegraph.svg](docs/benchmarks/flamegraph.svg).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing most of this section?

Comment on lines +1 to +11
//! # Cairo VM
//!
//! This crate provides a fast and safe implementation of the Cairo virtual machine in Rust.
//!
//! - [Project README](https://github.com/lambdaclass/cairo-vm/blob/main/README.md)
//! - [Extended documentation](https://github.com/lambdaclass/cairo-vm/blob/main/docs/README.md)
//!
//! ## Features
//! - STARK-friendly execution trace
//! - Custom hint processor support
//! - Tracing and debugging tools
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the links are correct, the rest might be a copy (or can be included) in the README.

Comment on lines +13 to +17
//! ## Example
//! ```rust
//! use cairo_vm::vm::vm_core::VirtualMachine;
//! // ... usage example ...
//! ```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example doesn't seem to be very useful IMO.

Comment on lines +19 to +20
//! ## Diagrams
//! Mermaid diagrams are supported in rustdoc via the `aquamarine` dependency.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean? Why is this here?

@MozirDmitriy
Copy link
Author

While the cross-links and links to benchmarks are useful, this PR doesn't address the corresponding issue:

  • This PR doesn't clean up or reorganize documentation.
  • It doesn't create a docs.rs following Cairo Native docs as a reference.
  • While the aquamarine dependency is added, it is not used.

I think the corresponding issue has to be divided into sub issues so we avoid having a huge PR that attempts to address this. What do you think?

Yes, i think you're right, maybe it would be really better to divide this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants